Skip to content

[py] complete test_should_throw_an_exception_if_an_alert_has_not_been_dealt_with_and_dismiss_the_alert #15559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Apr 16, 2025
Merged

[py] complete test_should_throw_an_exception_if_an_alert_has_not_been_dealt_with_and_dismiss_the_alert #15559

merged 8 commits into from
Apr 16, 2025

Conversation

Delta456
Copy link
Member

@Delta456 Delta456 commented Apr 3, 2025 •
edited by qodo-merge-pro bot
Loading

User description

💥 What does this PR do?

Implements alert test test_should_throw_an_exception_if_an_alert_has_not_been_dealt_with_and_dismiss_the_alert as it was marked as TODO

🔧 Implementation Notes

Uses code from other tests as inspiration

💡 Additional Considerations

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Tests

Description

  • Implemented the test test_should_throw_an_exception_if_an_alert_has_not_been_dealt_with_and_dismiss_the_alert.

  • Added logic to handle and dismiss alerts in the test.

  • Marked the test as expected to fail on Safari using @pytest.mark.xfail_safari.

Changes walkthrough 📝

Relevant files
Tests
alerts_tests.py
Added and completed alert handling test                                   

py/test/selenium/webdriver/common/alerts_tests.py

  • Completed the TODO test
    test_should_throw_an_exception_if_an_alert_has_not_been_dealt_with_and_dismiss_the_alert.
  • Added logic to trigger and handle alerts using WebDriverWait and
    exception handling.
  • Marked the test as expected to fail on Safari.
    		• +9/-3     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 3, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ No major issues detected

    @titusfortner titusfortner added the C-py Python Bindings label Apr 3, 2025
    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 3, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    ✅ Missing verification after alert dismissal
    Suggestion Impact:The commit added verification after alert dismissal, but instead of checking the page title as suggested, it verifies the alert is no longer present by trying to access alert.text which should raise NoAlertPresentException

    code diff:

    +    with pytest.raises(NoAlertPresentException):
+        alert.text

    The test is missing an assertion to verify that the alert was properly
    dismissed. After dismissing the alert, add an assertion to confirm that the
    alert is no longer present and that the page is in the expected state.

    py/test/selenium/webdriver/common/alerts_tests.py [187-196]

     @pytest.mark.xfail_safari
 def test_should_throw_an_exception_if_an_alert_has_not_been_dealt_with_and_dismiss_the_alert(driver, pages):
     pages.load("alerts.html")
     driver.find_element(By.ID, "alert").click()
     WebDriverWait(driver, 5).until(EC.alert_is_present())
 
     with pytest.raises(UnexpectedAlertPresentException):
         driver.find_element(By.ID, "select").click()
     alert = _wait_for_alert(driver)
     alert.dismiss()
+    
+    # Verify alert is dismissed
+    assert "Testing Alerts" == driver.title

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that the test lacks verification after dismissing the alert. Adding an assertion to check the page title is a good practice to ensure the alert was properly dismissed and the page returned to its expected state.

    		Medium
    • Update

    @VietND96 VietND96 requested a review from titusfortner April 3, 2025 13:01
    @VietND96
    Copy link
    Member

    VietND96 commented Apr 3, 2025

    LGTM, adding @titusfortner to double check.

    titusfortner
    titusfortner reviewed Apr 4, 2025
    View reviewed changes
    @Delta456 Delta456 requested a review from titusfortner April 7, 2025 09:17
    @cgoldberg cgoldberg self-requested a review April 15, 2025 21:40
    cgoldberg
    cgoldberg approved these changes Apr 15, 2025
    View reviewed changes
    Copy link
    Contributor

    @cgoldberg cgoldberg left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM! 👍🏼

    @cgoldberg cgoldberg merged commit 94aec15 into SeleniumHQ:trunk Apr 16, 2025
    17 checks passed
    @Delta456 Delta456 deleted the alert_tests branch April 16, 2025 06:12
    @garrettgregor garrettgregor mentioned this pull request May 3, 2025
    Merged
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @cgoldberg cgoldberg cgoldberg approved these changes

    @VietND96 VietND96 VietND96 approved these changes

    @titusfortner titusfortner Awaiting requested review from titusfortner

    Assignees
    No one assigned
    Labels
    C-py Python Bindings Review effort 1/5
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    4 participants
    @Delta456 @VietND96 @titusfortner @cgoldberg